code review 74790043: runtime: use VEH, not SEH, for windows/386 exception ha...

runtime: use VEH, not SEH, for windows/386 exception handling

Structured Exception Handling (SEH) was the first way to handle
exceptions (memory faults, divides by zero) on Windows.
The S might as well stand for "stack-based": the implementation
interprets stack addresses in a few different ways, and it gets
subtly confused by Go's management of stacks. It's also something
that requires active maintenance during cgo switches, and we've
had bugs in that maintenance in the past.

We have recently come to believe that SEH cannot work with
Go's stack usage. See http://golang.org/issue/7325 for details.

Vectored Exception Handling (VEH) is more like a Unix signal
handler: you set it once for the whole process and forget about it.

This CL drops all the SEH code and replaces it with VEH code.
Many special cases and 7 #ifdefs disappear.

VEH was introduced in Windows XP, so Go on windows/386 will
now require Windows XP or later. The previous requirement was
Windows 2000 or later. Windows 2000 immediately preceded
Windows XP, so Windows 2000 is the only affected version.
Microsoft stopped supporting Windows 2000 in 2010.
See http://golang.org/s/win2000-golang-nuts for details.

Fixes issue 7325.

[rsc, 2014-03-12 18:38:31]

Hello golang-codereviews@googlegroups.com (cc: iant, r),

I'd like you to review this change to
https://code.google.com/p/go/

[brainman, 2014-03-12 23:46:59]

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

https://codereview.appspot.com/74790043/diff/40001/src/pkg/runtime/os_windows...
src/pkg/runtime/os_windows.c:24: #pragma dynimport
runtime·GetQueuedCompletionStatusEx GetQueuedCompletionStatusEx "kernel32.dll"
I don't have this function in my kernel32.dll. Apparently you must have "Windows
Vista" for that. Remove otherwise none of Go built programs can run.

[slimsag, 2014-03-13 00:46:32]

On 2014/03/12 23:46:59, brainman wrote:
>
https://codereview.appspot.com/74790043/diff/40001/src/pkg/runtime/os_windows.c
> File src/pkg/runtime/os_windows.c (right):
> 
>
https://codereview.appspot.com/74790043/diff/40001/src/pkg/runtime/os_windows...
> src/pkg/runtime/os_windows.c:24: #pragma dynimport
> runtime·GetQueuedCompletionStatusEx GetQueuedCompletionStatusEx "kernel32.dll"
> I don't have this function in my kernel32.dll. Apparently you must have
"Windows
> Vista" for that. Remove otherwise none of Go built programs can run.

GetQueuedCompletionStatus (no Ex on the end) is XP compatible, perhaps that can
be used.

[brainman, 2014-03-13 00:48:03]

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

https://codereview.appspot.com/74790043/diff/40001/src/pkg/runtime/os_windows...
src/pkg/runtime/os_windows.c:95:
runtime·stdcall(runtime·AddVectoredExceptionHandler, 2, (uintptr)1,
(uintptr)runtime·sigtramp);
This will call windows/amd64 runtime.sigtramp too. I don't think you've
accounted for that. You should disable old windows/amd64 exception handler
invocation and use new way just like you did with windows/386. Windows/amd64
build crashes here. Did you try running it on windows/amd64?

https://codereview.appspot.com/74790043/diff/40001/src/pkg/runtime/sys_window...
File src/pkg/runtime/sys_windows_386.s (right):

https://codereview.appspot.com/74790043/diff/40001/src/pkg/runtime/sys_window...
src/pkg/runtime/sys_windows_386.s:119: CALL	runtime·sighandler(SB)
Please, leave old comment of

// AX is set to report result back to Windows

in. So we don't forget AX value is important.

https://codereview.appspot.com/74790043/diff/40001/src/pkg/runtime/sys_window...
src/pkg/runtime/sys_windows_386.s:130: BYTE $0xC2; WORD $4
I would just do what ctrlhandler and profileloop do.

[brainman, 2014-03-13 00:53:58]

On 2014/03/13 00:46:32, stephen.gutekanst wrote:
> 
> ..., perhaps that can
> be used.

No. GetQueuedCompletionStatus won't do, we need GetQueuedCompletionStatusEx. See
netpoll_windows.c for details.

Alex

[dave_cheney.net, 2014-03-13 01:04:25]

> No. GetQueuedCompletionStatus won't do, we need
> GetQueuedCompletionStatusEx. See netpoll_windows.c for details.

I'm confused. If

[dave_cheney.net, 2014-03-13 01:04:53]

>> No. GetQueuedCompletionStatus won't do, we need
>> GetQueuedCompletionStatusEx. See netpoll_windows.c for details.
>
> I'm confused. If

I'm confused. If GetQueuedCompletionStatusEx is required, then Go
never supported Windows 2000, or XP for that matter.

[brainman, 2014-03-13 01:07:31]

On 2014/03/13 01:04:53, dfc wrote:
> >> No. GetQueuedCompletionStatus won't do, we need
> >> GetQueuedCompletionStatusEx. See netpoll_windows.c for details.
> >
> > I'm confused. If
> 
> I'm confused. If GetQueuedCompletionStatusEx is required, then Go
> never supported Windows 2000, or XP for that matter.

GetQueuedCompletionStatusEx is an optional functionality that we use if
available. We try and load GetQueuedCompletionStatusEx from kernel32.dll - if
not found we use different program path.

Alex

[dave_cheney.net, 2014-03-13 01:15:47]

Right. Ignore me, carry on.

On Thu, Mar 13, 2014 at 12:07 PM,  <alex.brainman@gmail.com> wrote:
> On 2014/03/13 01:04:53, dfc wrote:
>>
>> >> No. GetQueuedCompletionStatus won't do, we need
>> >> GetQueuedCompletionStatusEx. See netpoll_windows.c for details.
>> >
>> > I'm confused. If
>
>
>> I'm confused. If GetQueuedCompletionStatusEx is required, then Go
>> never supported Windows 2000, or XP for that matter.
>
>
> GetQueuedCompletionStatusEx is an optional functionality that we use if
> available. We try and load GetQueuedCompletionStatusEx from kernel32.dll
> - if not found we use different program path.
>
> Alex
>
> https://codereview.appspot.com/74790043/

[brainman, 2014-03-13 01:26:59]

I've reverted code related to GetQueuedCompletionStatusEx, and I can proceed to
testing now. But it fails:

...
ok      math/cmplx      0.859s
ok      math/rand       0.469s
ok      mime    0.109s
ok      mime/multipart  0.406s
Exception 0x6d9 0x1003f 0x0 0x7c812afb
PC=0x7c812afb
signal arrived during cgo execution

syscall.Syscall(0x71ab4480, 0x3, 0x6d8, 0x10d5c028, 0x10, ...)
        C:/go/root/src/pkg/runtime/syscall_windows.goc:55 +0x4b
syscall.bind(0x6d8, 0x10d5c028, 0x10, 0x0, 0x0)
        C:/go/root/src/pkg/syscall/zsyscall_windows_386.go:1327 +0x85
syscall.Bind(0x6d8, 0x3d04b0, 0x10d5c020, 0x0, 0x0)
        C:/go/root/src/pkg/syscall/syscall_windows.go:682 +0x95
net.(*netFD).listenDatagram(0x10d1e000, 0x3d1730, 0x10d5c000, 0x692dec, 0x0,
...)
        C:/go/root/src/pkg/net/sock_posix.go:188 +0x1f2
net.socket(0x5c4dd0, 0x3, 0x2, 0x2, 0x0, ...)
        C:/go/root/src/pkg/net/sock_posix.go:84 +0x247
net.internetSocket(0x5c4dd0, 0x3, 0x3d1730, 0x73fd50, 0x0, ...)
        C:/go/root/src/pkg/net/ipsock_posix.go:136 +0x106
net.ListenMulticastUDP(0x5c4dd0, 0x3, 0x0, 0x73fd50, 0x0, ...)
        C:/go/root/src/pkg/net/udpsock_posix.go:221 +0x13a
net.TestIPv4MulticastListener(0x10d2bce0)
        C:/go/root/src/pkg/net/multicast_test.go:54 +0x2ba
testing.tRunner(0x10d2bce0, 0x747978)
        C:/go/root/src/pkg/testing/testing.go:422 +0x94
created by testing.RunTests
        C:/go/root/src/pkg/testing/testing.go:503 +0x70b

goroutine 16 [chan receive]:
testing.RunTests(0x692acc, 0x7476c0, 0x73, 0x73, 0x1)
        C:/go/root/src/pkg/testing/testing.go:504 +0x731
testing.Main(0x692acc, 0x7476c0, 0x73, 0x73, 0x742940, ...)
        C:/go/root/src/pkg/testing/testing.go:435 +0x77
main.main()
       
C:/DOCUME~1/brainman/LOCALS~1/Temp/go-build785780803/net/_test/_testmain.go:309
+0x82

goroutine 21 [chan receive]:
net.(*ioSrv).ProcessRemoteIO(0x10d18270)
        C:/go/root/src/pkg/net/fd_windows.go:144 +0x6c
created by net.startServer
        C:/go/root/src/pkg/net/fd_windows.go:244 +0x87

goroutine 22 [chan receive]:
net.(*ioSrv).ProcessRemoteIO(0x10d18278)
        C:/go/root/src/pkg/net/fd_windows.go:144 +0x6c
created by net.startServer
        C:/go/root/src/pkg/net/fd_windows.go:246 +0xc0
eax     0x3105f988
ebx     0x1
ecx     0x8b1f
edx     0x95120
edi     0x662b7248
esi     0x0
ebp     0x3105f9d8
esp     0x3105f984
eip     0x7c812afb
eflags  0x246
cs      0x1b
fs      0x3b
gs      0x0
FAIL    net     0.328s
...

Alex

[rsc, 2014-03-13 01:59:59]

Can you try changing runtime.sighandler to add:

if(info->ExceptionCode < 0x80000000)
    return -1;

and see if the problem goes away? That's not the right fix but it would
tell us whether this is another one of those "informational" exceptions
like DBG_PRINTEXCEPTION_C.

Thanks.
Russ

[brainman, 2014-03-13 04:27:52]

On 2014/03/13 01:59:59, rsc wrote:
> Can you try changing runtime.sighandler ...

Tried that. Different error:

...
ok      mime    0.109s
ok      mime/multipart  0.375s
unexpected fault address 0x77ef560f
fatal error: fault
[signal 0xc0000005 code=0x1 addr=0x77ef560f pc=0x77e9fc6a]

runtime stack:
runtime: unexpected return pc for runtime.sigpanic called from 0x77e9fc6a
runtime.throw(0x743be6)
        C:/go/root/src/pkg/runtime/panic.c:496 +0x67
runtime: unexpected return pc for runtime.sigpanic called from 0x77e9fc6a
runtime.sigpanic()
        C:/go/root/src/pkg/runtime/os_windows.c:357 +0xc5

goroutine 105 [syscall]:
runtime.cgocall(0x428a90, 0x31081c50)
        C:/go/root/src/pkg/runtime/cgocall.c:143 +0xdb fp=0x31081c48
syscall.Syscall(0x71ab4480, 0x3, 0x6b0, 0x10d5c028, 0x10, ...)
        C:/go/root/src/pkg/runtime/syscall_windows.goc:55 +0x4b fp=0x31081c6c
syscall.bind(0x6b0, 0x10d5c028, 0x10, 0x0, 0x0)
        C:/go/root/src/pkg/syscall/zsyscall_windows_386.go:1327 +0x85
fp=0x31081ca0
syscall.Bind(0x6b0, 0x3d04b0, 0x10d5c020, 0x0, 0x0)
        C:/go/root/src/pkg/syscall/syscall_windows.go:682 +0x95 fp=0x31081cb8
net.(*netFD).listenDatagram(0x10d1e000, 0x3d1730, 0x10d5c000, 0x692e0c, 0x0,
...)
        C:/go/root/src/pkg/net/sock_posix.go:188 +0x1f2 fp=0x31081d48
net.socket(0x5c4df0, 0x3, 0x2, 0x2, 0x0, ...)
        C:/go/root/src/pkg/net/sock_posix.go:84 +0x247 fp=0x31081d9c
net.internetSocket(0x5c4df0, 0x3, 0x3d1730, 0x73fd50, 0x0, ...)
        C:/go/root/src/pkg/net/ipsock_posix.go:136 +0x106 fp=0x31081dec
net.ListenMulticastUDP(0x5c4df0, 0x3, 0x0, 0x73fd50, 0x0, ...)
        C:/go/root/src/pkg/net/udpsock_posix.go:221 +0x13a fp=0x31081e8c
net.TestIPv4MulticastListener(0x10d2bce0)
        C:/go/root/src/pkg/net/multicast_test.go:54 +0x2ba fp=0x31081f94
testing.tRunner(0x10d2bce0, 0x747978)
        C:/go/root/src/pkg/testing/testing.go:422 +0x94 fp=0x31081fc8
runtime.goexit()
        C:/go/root/src/pkg/runtime/proc.c:1426 fp=0x31081fcc
created by testing.RunTests
        C:/go/root/src/pkg/testing/testing.go:503 +0x70b

goroutine 16 [chan receive]:
testing.RunTests(0x692aec, 0x7476c0, 0x73, 0x73, 0x1)
        C:/go/root/src/pkg/testing/testing.go:504 +0x731
testing.Main(0x692aec, 0x7476c0, 0x73, 0x73, 0x742940, ...)
        C:/go/root/src/pkg/testing/testing.go:435 +0x77
main.main()
       
C:/DOCUME~1/brainman/LOCALS~1/Temp/go-build402068915/net/_test/_testmain.go:309
+0x82

goroutine 21 [chan receive]:
net.(*ioSrv).ProcessRemoteIO(0x10d18270)
        C:/go/root/src/pkg/net/fd_windows.go:144 +0x6c
created by net.startServer
        C:/go/root/src/pkg/net/fd_windows.go:244 +0x87

goroutine 22 [chan receive]:
net.(*ioSrv).ProcessRemoteIO(0x10d18278)
        C:/go/root/src/pkg/net/fd_windows.go:144 +0x6c
created by net.startServer
        C:/go/root/src/pkg/net/fd_windows.go:246 +0xc0
FAIL    net     0.328s
...

Alex

[rsc, 2014-03-13 17:47:08]

Hi Alex,

I've done some more playing on the builder but I can't make this exact
failure happen. Could you try commenting out the line "JMP skipcheckpc" in
runtime/sys_windows_386.s and see if things get better?

Thanks.
Russ

[brainman, 2014-03-14 06:04:21]

On 2014/03/13 17:47:08, rsc wrote:
> ... Could you try commenting out the line "JMP skipcheckpc" in
> runtime/sys_windows_386.s and see if things get better?
> 

It doesn't help. I have couple of different tests crashing in different
packages. I picked the simplest one - os/user. The test calls TranslateName
Windows API, and I can see an exception happens inside the call with one of
those "info" messages (ExceptionCode < 0x80000000), so I ignore it (return 0).
But that immediately follows by C0000005 (we're still inside of TranslateName
Windows API call), and I have no choice but to bail out. So you see error
message similar to #12. I tried everything I can think of: skipping exceptions
from non-Go addresses, returning both 0 and -1 from our exception handler,
putting our exception handler first and last on the exception handlers list. All
with no success.

And then I saw this
http://blogs.msdn.com/b/zhanli/archive/2010/06/25/c-tips-addvectoredexception...,
so I decided to use SetUnhandledExceptionFilter instead (see my diff here
https://code.google.com/p/go/issues/detail?id=7325#c30). With
SetUnhandledExceptionFilter everything works - all.bat completes, except
sometimes divmod.go test fails like:

C:\go\root\test>go run run.go -- divmod.go
# go run run.go -- divmod.go
incorrect output
exit status -1073741676

FAIL    divmod.go       0.172s
exit status 1

Also SetUnhandledExceptionFilter does not work while debugging is on (so I'm not
sure it is acceptable). On the other hand SetUnhandledExceptionFilter handler
does not see any "info" exceptions, so that is a good thing.

Not sure what to do next. Debugging is hard even if it works - all source files
and line numbers are wrong.

Alex

[rsc, 2014-03-14 14:30:10]

On Fri, Mar 14, 2014 at 2:04 AM, <alex.brainman@gmail.com> wrote:

> It doesn't help. I have couple of different tests crashing in different
> packages. I picked the simplest one - os/user. The test calls
> TranslateName Windows API, and I can see an exception happens inside the
> call with one of those "info" messages (ExceptionCode < 0x80000000), so
> I ignore it (return 0). But that immediately follows by C0000005 (we're
> still inside of TranslateName Windows API call), and I have no choice
> but to bail out. So you see error message similar to #12. I tried
> everything I can think of: skipping exceptions from non-Go addresses,
> returning both 0 and -1 from our exception handler, putting our
> exception handler first and last on the exception handlers list. All
> with no success.
>
> And then I saw this
> http://blogs.msdn.com/b/zhanli/archive/2010/06/25/c-tips-
> addvectoredexceptionhandler-addvectoredcontinuehandler-and-
> setunhandledexceptionfilter.aspx,
> so I decided to use SetUnhandledExceptionFilter instead (see my diff
> here https://code.google.com/p/go/issues/detail?id=7325#c30). With
> SetUnhandledExceptionFilter everything works - all.bat completes, except
> sometimes divmod.go test fails like:
>
> C:\go\root\test>go run run.go -- divmod.go
> # go run run.go -- divmod.go
> incorrect output
> exit status -1073741676
>
> FAIL    divmod.go       0.172s
> exit status 1
>
> Also SetUnhandledExceptionFilter does not work while debugging is on (so
> I'm not sure it is acceptable). On the other hand
> SetUnhandledExceptionFilter handler does not see any "info" exceptions,
> so that is a good thing.
>

I'm happy to add SetUnhandledExceptionFilter. What do you mean by "it does
not work while debugging is on"?
The exit status there is 0xC0000094 which is EXCEPTION_INT_DIVIDE_BY_ZERO,
so it looks like we are losing the handler sporadically?

I'd like to submit this CL as is and then we can work on patches on top of
it to get it working everywhere.

Thanks.
Russ

[brainman, 2014-03-15 01:09:32]

On 2014/03/14 14:30:10, rsc wrote:
> 
> I'm happy to add SetUnhandledExceptionFilter. What do you mean by "it does
> not work while debugging is on"?

The exception handler installed by SetUnhandledExceptionFilter is not called, if
program is debugged (I can actually see that). Search for "debugged" on
http://msdn.microsoft.com/en-us/library/windows/desktop/ms680634(v=vs.85).aspx\
Also search for "debug" on
http://blogs.msdn.com/b/zhanli/archive/2010/06/25/c-tips-addvectoredexception...

> The exit status there is 0xC0000094 which is EXCEPTION_INT_DIVIDE_BY_ZERO,
> so it looks like we are losing the handler sporadically?

Yes. Sometimes it works, sometimes it does not.

> I'd like to submit this CL as is and then we can work on patches on top of
> it to get it working everywhere.

I am far from convinced this CL's approach is working better then our current
approach, but

LGTM

I still think perhaps our current exception handler can be improved. Maybe we're
doing too much inside of our exception handler. None of our functions is
NOSPLIT, perhaps we should not allocate memory inside of our exception handler.
It can be rewritten to be much simple. I also wonder what happens, if exception
happens inside of our exception handler. Perhaps what we see in divmod.exe is
result of that.

Anyway, are you going to address any issues I raised in #2 and #4?

Alex

[brainman, 2014-03-17 06:10:02]

On 2014/03/15 01:09:32, brainman wrote:
> On 2014/03/14 14:30:10, rsc wrote:
> > 
> > ... What do you mean by "it does
> > not work while debugging is on"?
> 

Another reference http://support.microsoft.com/kb/173652

Alex

[rsc, 2014-03-25 00:35:15]

Can someone please LGTM this? Yes, it breaks Alex's system (which OS, by
the way?) but I'd like to get it in to deflake the builder and then we can
work out what's wrong on Alex's system.

Thanks.

[r, 2014-03-25 00:37:58]

You have an LGTM from alex.

-rob

[brainman, 2014-03-25 00:38:28]

On 2014/03/25 00:35:15, rsc wrote:
> ... which OS, by
> the way? ...

windows xp

Alex

[rsc, 2014-03-25 01:22:21]

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

runtime: use VEH, not SEH, for windows/386 exception handling

Structured Exception Handling (SEH) was the first way to handle
exceptions (memory faults, divides by zero) on Windows.
The S might as well stand for "stack-based": the implementation
interprets stack addresses in a few different ways, and it gets
subtly confused by Go's management of stacks. It's also something
that requires active maintenance during cgo switches, and we've
had bugs in that maintenance in the past.

We have recently come to believe that SEH cannot work with
Go's stack usage. See http://golang.org/issue/7325 for details.

Vectored Exception Handling (VEH) is more like a Unix signal
handler: you set it once for the whole process and forget about it.

This CL drops all the SEH code and replaces it with VEH code.
Many special cases and 7 #ifdefs disappear.

VEH was introduced in Windows XP, so Go on windows/386 will
now require Windows XP or later. The previous requirement was
Windows 2000 or later. Windows 2000 immediately preceded
Windows XP, so Windows 2000 is the only affected version.
Microsoft stopped supporting Windows 2000 in 2010.
See http://golang.org/s/win2000-golang-nuts for details.

Fixes issue 7325.

LGTM=alex.brainman, r
R=golang-codereviews, alex.brainman, stephen.gutekanst, dave
CC=golang-codereviews, iant, r
https://codereview.appspot.com/74790043

[gobot, 2014-03-25 01:40:17]

This CL appears to have broken the windows-amd64-race builder.
See http://build.golang.org/log/e9fd41a55f58a9c486649a9474e64e34a96ecc9c

[brainman, 2014-03-25 03:12:44]

Sorry for late notice.

Alex

https://codereview.appspot.com/74790043/diff/60001/src/pkg/runtime/sys_window...
File src/pkg/runtime/sys_windows_386.s (right):

https://codereview.appspot.com/74790043/diff/60001/src/pkg/runtime/sys_window...
src/pkg/runtime/sys_windows_386.s:79: MOVL	0(DI), BX // ExceptionRecord*
Should you be using BX here, if it is callee-saved register?

[brainman, 2014-03-25 03:17:46]

On 2014/03/25 03:12:44, brainman wrote:
> Should you be using BX here, if it is callee-saved register?

Same for DI.

Alex

[rsc, 2014-03-25 14:58:05]

https://codereview.appspot.com/74790043/diff/60001/src/pkg/runtime/sys_window...
File src/pkg/runtime/sys_windows_386.s (right):

https://codereview.appspot.com/74790043/diff/60001/src/pkg/runtime/sys_window...
src/pkg/runtime/sys_windows_386.s:79: MOVL	0(DI), BX // ExceptionRecord*
On 2014/03/25 03:12:45, brainman wrote:
> Should you be using BX here, if it is callee-saved register?

Good point. Probably not. Perhaps that is causing the problem on your system?

[brainman, 2014-03-26 06:08:56]

On 2014/03/25 14:58:05, rsc wrote:
> 
> ... Perhaps that is causing the problem on your system?

Unfortunately not. But I've found a fix for my windows xp
https://codereview.appspot.com/80400043/. It also fixes BX/DI usage problem. So,
I think we are finally as good as we can be.

Alex