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

[stephen....@gmail.com]

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.c#newcode24
> 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.

https://codereview.appspot.com/74790043/

[alex.b...@gmail.com]

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.c#newcode95
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_windows_386.s
File src/pkg/runtime/sys_windows_386.s (right):

https://codereview.appspot.com/74790043/diff/40001/src/pkg/runtime/sys_windows_386.s#newcode119
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_windows_386.s#newcode130
src/pkg/runtime/sys_windows_386.s:130: BYTE $0xC2; WORD $4
I would just do what ctrlhandler and profileloop do.

https://codereview.appspot.com/74790043/

[alex.b...@gmail.com]

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

https://codereview.appspot.com/74790043/

[Dave Cheney]

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

I'm confused. If

[Dave Cheney]

>> 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.

[alex.b...@gmail.com]

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/

[Dave Cheney]

Right. Ignore me, carry on.

[alex.b...@gmail.com]

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

https://codereview.appspot.com/74790043/

[Russ Cox]

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

[alex.b...@gmail.com]

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

[Russ Cox]

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

[alex.b...@gmail.com]

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-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.

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

Alex

https://codereview.appspot.com/74790043/

[Russ Cox]

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

[alex.b...@gmail.com]

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-addvectoredexceptionhandler-addvectoredcontinuehandler-and-setunhandledexceptionfilter.aspx

> 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

https://codereview.appspot.com/74790043/

[alex.b...@gmail.com]

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

https://codereview.appspot.com/74790043/

[Russ Cox]

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.

[Rob Pike]

You have an LGTM from alex.

-rob

[alex.b...@gmail.com]

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

windows xp

Alex

https://codereview.appspot.com/74790043/

[r...@golang.org]

*** 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


https://codereview.appspot.com/74790043/

[go...@golang.org]

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

https://codereview.appspot.com/74790043/

[alex.b...@gmail.com]

Sorry for late notice.

Alex


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

https://codereview.appspot.com/74790043/diff/60001/src/pkg/runtime/sys_windows_386.s#newcode79
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?

https://codereview.appspot.com/74790043/

[alex.b...@gmail.com]

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

https://codereview.appspot.com/74790043/

[r...@golang.org]

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

https://codereview.appspot.com/74790043/diff/60001/src/pkg/runtime/sys_windows_386.s#newcode79
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?

https://codereview.appspot.com/74790043/

[alex.b...@gmail.com]

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

https://codereview.appspot.com/74790043/