code review 145150043: runtime: handle all windows exception

runtime: handle all windows exception

Fixes issue 8006.

[brainman, 2014-09-19 02:26:29]

Hello golang-codereviews@googlegroups.com,

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

[brainman, 2014-09-19 02:26:49]

I tried to change as little code as possible.

Alex

[brainman, 2014-10-01 07:29:33]

ping

[rsc, 2014-10-08 03:49:21]

LGTM

[brainman, 2014-10-09 05:52:35]

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

runtime: handle all windows exception

Fixes issue 8006.

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

[gobot, 2014-10-09 05:57:24]

This CL appears to have broken the windows-386 builder.
See http://build.golang.org/log/bd4a48327458ed28699e3d08338ecc0270f6a0b5

[brainman, 2014-10-09 06:30:24]

On 2014/10/09 05:57:24, gobot wrote:
> This CL appears to have broken the windows-386 builder.
> See http://build.golang.org/log/bd4a48327458ed28699e3d08338ecc0270f6a0b5

Reverted, because too many unexplained errors. Like:

*** Test killed: ran too long (3m0s).
FAIL	html/template	180.266s

Is there any way I can access windows builder to see what is going on there?
Maybe similar computer.

Thank you.

Alex

[gobot, 2014-10-09 07:32:12]

This changed caused perf changes on windows-amd64-perf:


json-1                    old          new      delta
cputime             178750000    184687500      +3.32
time                178701138    184926752      +3.48

json-2                    old          new      delta
cputime             180781250    187343750      +3.63
time                 90952008     93904671      +3.25

json-4                    old          new      delta
cputime             184062500    189296875      +2.84
time                 46541967     47898190      +2.91

json-8                    old          new      delta
time                 24762327     25392896      +2.55

http://build.golang.org/perfdetail?commit=8b3d26697b8db9c738fbd96ee381e69ab92...



—gobot

[bradfitz, 2014-10-09 08:56:43]

You can make your own identical VM.

The doc with instructions was mailed out or is linked from the
github.com/golang/winstrap page.
 On Oct 9, 2014 8:30 AM, <alex.brainman@gmail.com> wrote:

> On 2014/10/09 05:57:24, gobot wrote:
>
>> This CL appears to have broken the windows-386 builder.
>> See
>>
> http://build.golang.org/log/bd4a48327458ed28699e3d08338ecc0270f6a0b5
>
> Reverted, because too many unexplained errors. Like:
>
> *** Test killed: ran too long (3m0s).
> FAIL    html/template   180.266s
>
> Is there any way I can access windows builder to see what is going on
> there? Maybe similar computer.
>
> Thank you.
>
> Alex
>
> https://codereview.appspot.com/145150043/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[brainman, 2014-10-09 09:33:11]

On 2014/10/09 08:56:43, bradfitz wrote:
> You can make your own identical VM.
> 
> The doc with instructions was mailed out or is linked from the
> github.com/golang/winstrap page.

I will try that. Thank you.

Alex

[brainman, 2014-10-13 06:04:12]

rsc,

I have found some problems with this change. Our windows-368 builder is actually
running on 64-bit OS. I didn't account for that - I always test windows-386
build on 32-bit OS and windows-amd64 on 64-bit. Once I tried running
8b3d26697b8d windows-386 on 64-bit OS, I could see all that breakage.

There are 2 problems:

1) I assumed that AddVectoredContinueHandler API does not exist on windows-386,
but it is available for windows-386 program running on 64-bit OS. We should
stick to using SetUnhandledExceptionFilter for windows-386 everywhere, because I
get some unusual unhanded exceptions when I use AddVectoredContinueHandler.

2) Now that I am running windows-386 on 64-bit, I can also see TestBreackpoint
crashing with some unusual unhanded exception, because we ignore
EXCEPTION_BREAKPOINT in our exception handler. Perhaps what we see in issue 8796
occasionally. I propose we include EXCEPTION_BREAKPOINT in our
runtime·issigpanic, so no other code gets to see it, and we just crash with
standard stack trace.

I've applied this:

diff --git a/src/runtime/os_windows.c b/src/runtime/os_windows.c
--- a/src/runtime/os_windows.c
+++ b/src/runtime/os_windows.c
@@ -119,7 +119,7 @@
 	runtime·stdcall2(runtime·AddVectoredExceptionHandler, 1,
(uintptr)runtime·exceptiontramp);
 	if(kernel32 != nil)
 		addVectoredContinueHandler = runtime·stdcall2(runtime·GetProcAddress,
(uintptr)kernel32, (uintptr)"AddVectoredContinueHandler");
-	if(addVectoredContinueHandler == nil)
+	if(addVectoredContinueHandler == nil || sizeof(void*) == 4)
 		// use SetUnhandledExceptionFilter if VectoredContinueHandler is unavailable.
 		// note: SetUnhandledExceptionFilter handler won't be called, if debugging.
 		runtime·stdcall1(runtime·SetUnhandledExceptionFilter,
(uintptr)runtime·lastcontinuetramp);
@@ -474,6 +474,10 @@
 	runtime·usleep1(10*us);
 }
 
+enum {
+	EXCEPTION_BREAKPOINT            = 0x80000003,
+};
+
 uint32
 runtime·issigpanic(uint32 code)
 {
@@ -486,6 +490,7 @@
 	case EXCEPTION_FLT_INEXACT_RESULT:
 	case EXCEPTION_FLT_OVERFLOW:
 	case EXCEPTION_FLT_UNDERFLOW:
+	case EXCEPTION_BREAKPOINT:
 		return 1;
 	}
 	return 0;

to 8b3d26697b8d, and all builds complete here - windows-386 on 32-bit, and
windows-386 and windows-amd64 on 64-bit.

It would be simple change, but our current tip looks different now. I had to
undo 8b3d26697b8d (commit 22318cd31d7d) to make builders green - I didn't know
what the problem was.

I could send single CL that contains 8b3d26697b8d changes plus change above. Or
I could send them separately:
1) disable TestBreakpoint on windows;
2) undo 22318cd31d7d (it is undo of undo of 8b3d26697b8d) - windows-386 will be
broken here;
3) one line change to always use SetUnhandledExceptionFilter on windows-386 -
all builds should be green now;
4) handle EXCEPTION_BREAKPOINT in Go exception handler, and enable
TestBreakpoint on windows again.

What do you think?

Alex

[minux, 2014-10-13 06:23:09]

On Mon, Oct 13, 2014 at 2:04 AM,  <alex.brainman@gmail.com> wrote:
> I have found some problems with this change. Our windows-368 builder is
> actually running on 64-bit OS. I didn't account for that
So we should add another windows/386 builder using 32-bit 2003 or xp.

[brainman, 2014-10-13 06:26:49]

On 2014/10/13 06:23:09, minux wrote:
> So we should add another windows/386 builder using 32-bit 2003 or xp.

Hopefully 32-bit OS is not as popular as before.
I am running xp. I will be your builder. :-)

Alex

[bradfitz, 2014-10-13 10:43:03]

Is 32-bit Windows on GCE an option?


On Mon, Oct 13, 2014 at 8:26 AM, <alex.brainman@gmail.com> wrote:

> On 2014/10/13 06:23:09, minux wrote:
>
>> So we should add another windows/386 builder using 32-bit 2003 or xp.
>>
>
> Hopefully 32-bit OS is not as popular as before.
> I am running xp. I will be your builder. :-)
>
> Alex
>
> https://codereview.appspot.com/145150043/
>

[brainman, 2014-10-13 11:16:20]

On 2014/10/13 10:43:03, bradfitz wrote:
> Is 32-bit Windows on GCE an option?

I don't believe so. But I really don't now.

Alex

[brainman, 2014-10-14 01:43:46]

New "all in one" fix https://codereview.appspot.com/155360043/. But I am still
happy to split it as described above.

Alex

[rsc, 2014-10-14 14:26:04]

sending one big CL is fine; please do it soon.

[brainman, 2014-10-14 20:19:45]

On 2014/10/14 14:26:04, rsc wrote:
> sending one big CL is fine; please do it soon.

I did. https://codereview.appspot.com/155360043/

Alex