Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1065)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 8 months ago by rsc
Modified:
3 years, 8 months ago
Reviewers:
r, gobot, brainman
CC:
golang-codereviews, slimsag, dfc, iant, r
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : diff -r b1e34388854e https://code.google.com/p/go/ #

Patch Set 3 : diff -r 6960b48414c9 https://code.google.com/p/go/ #

Total comments: 4

Patch Set 4 : diff -r f60620ad71fa https://code.google.com/p/go/ #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+95 lines, -190 lines) Patch
M src/pkg/runtime/asm_386.s View 1 1 chunk +0 lines, -1 line 0 comments Download
M src/pkg/runtime/cgocall.c View 1 3 chunks +0 lines, -12 lines 0 comments Download
M src/pkg/runtime/export_test.go View 1 1 chunk +0 lines, -4 lines 0 comments Download
M src/pkg/runtime/os_windows.c View 1 5 chunks +18 lines, -24 lines 0 comments Download
M src/pkg/runtime/os_windows_386.c View 1 2 3 chunks +30 lines, -9 lines 0 comments Download
M src/pkg/runtime/proc.c View 1 2 3 4 chunks +0 lines, -22 lines 0 comments Download
M src/pkg/runtime/runtime.h View 1 2 3 5 chunks +0 lines, -35 lines 0 comments Download
M src/pkg/runtime/runtime1.goc View 1 1 chunk +0 lines, -4 lines 0 comments Download
M src/pkg/runtime/sys_windows_386.s View 1 2 5 chunks +47 lines, -64 lines 2 comments Download
M src/pkg/runtime/syscall_windows_test.go View 1 3 chunks +0 lines, -15 lines 0 comments Download

Messages

Total messages: 26
rsc
Hello golang-codereviews@googlegroups.com (cc: iant, r), I'd like you to review this change to https://code.google.com/p/go/
3 years, 8 months ago (2014-03-12 18:38:31 UTC) #1
brainman
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 ...
3 years, 8 months ago (2014-03-12 23:46:59 UTC) #2
slimsag
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 > ...
3 years, 8 months ago (2014-03-13 00:46:32 UTC) #3
brainman
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 ...
3 years, 8 months ago (2014-03-13 00:48:03 UTC) #4
brainman
On 2014/03/13 00:46:32, stephen.gutekanst wrote: > > ..., perhaps that can > be used. No. ...
3 years, 8 months ago (2014-03-13 00:53:58 UTC) #5
dfc
> No. GetQueuedCompletionStatus won't do, we need > GetQueuedCompletionStatusEx. See netpoll_windows.c for details. I'm confused. ...
3 years, 8 months ago (2014-03-13 01:04:25 UTC) #6
dfc
>> No. GetQueuedCompletionStatus won't do, we need >> GetQueuedCompletionStatusEx. See netpoll_windows.c for details. > > ...
3 years, 8 months ago (2014-03-13 01:04:53 UTC) #7
brainman
On 2014/03/13 01:04:53, dfc wrote: > >> No. GetQueuedCompletionStatus won't do, we need > >> ...
3 years, 8 months ago (2014-03-13 01:07:31 UTC) #8
dfc
Right. Ignore me, carry on. On Thu, Mar 13, 2014 at 12:07 PM, <alex.brainman@gmail.com> wrote: ...
3 years, 8 months ago (2014-03-13 01:15:47 UTC) #9
brainman
I've reverted code related to GetQueuedCompletionStatusEx, and I can proceed to testing now. But it ...
3 years, 8 months ago (2014-03-13 01:26:59 UTC) #10
rsc
Can you try changing runtime.sighandler to add: if(info->ExceptionCode < 0x80000000) return -1; and see if ...
3 years, 8 months ago (2014-03-13 01:59:59 UTC) #11
brainman
On 2014/03/13 01:59:59, rsc wrote: > Can you try changing runtime.sighandler ... Tried that. Different ...
3 years, 8 months ago (2014-03-13 04:27:52 UTC) #12
rsc
Hi Alex, I've done some more playing on the builder but I can't make this ...
3 years, 8 months ago (2014-03-13 17:47:08 UTC) #13
brainman
On 2014/03/13 17:47:08, rsc wrote: > ... Could you try commenting out the line "JMP ...
3 years, 8 months ago (2014-03-14 06:04:21 UTC) #14
rsc
On Fri, Mar 14, 2014 at 2:04 AM, <alex.brainman@gmail.com> wrote: > It doesn't help. I ...
3 years, 8 months ago (2014-03-14 14:30:10 UTC) #15
brainman
On 2014/03/14 14:30:10, rsc wrote: > > I'm happy to add SetUnhandledExceptionFilter. What do you ...
3 years, 8 months ago (2014-03-15 01:09:32 UTC) #16
brainman
On 2014/03/15 01:09:32, brainman wrote: > On 2014/03/14 14:30:10, rsc wrote: > > > > ...
3 years, 8 months ago (2014-03-17 06:10:02 UTC) #17
rsc
Can someone please LGTM this? Yes, it breaks Alex's system (which OS, by the way?) ...
3 years, 8 months ago (2014-03-25 00:35:15 UTC) #18
r
You have an LGTM from alex. -rob
3 years, 8 months ago (2014-03-25 00:37:58 UTC) #19
brainman
On 2014/03/25 00:35:15, rsc wrote: > ... which OS, by > the way? ... windows ...
3 years, 8 months ago (2014-03-25 00:38:28 UTC) #20
rsc
*** Submitted as https://code.google.com/p/go/source/detail?r=0a3722aa9092 *** runtime: use VEH, not SEH, for windows/386 exception handling Structured ...
3 years, 8 months ago (2014-03-25 01:22:21 UTC) #21
gobot
This CL appears to have broken the windows-amd64-race builder. See http://build.golang.org/log/e9fd41a55f58a9c486649a9474e64e34a96ecc9c
3 years, 8 months ago (2014-03-25 01:40:17 UTC) #22
brainman
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 // ...
3 years, 8 months ago (2014-03-25 03:12:44 UTC) #23
brainman
On 2014/03/25 03:12:44, brainman wrote: > Should you be using BX here, if it is ...
3 years, 8 months ago (2014-03-25 03:17:46 UTC) #24
rsc
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 ...
3 years, 8 months ago (2014-03-25 14:58:05 UTC) #25
brainman
3 years, 8 months ago (2014-03-26 06:08:56 UTC) #26
Message was sent while issue was closed.
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld c59f3c4