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

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:
5 years, 1 month ago by rsc
Modified:
5 years 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/
5 years, 1 month 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 ...
5 years, 1 month 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 > ...
5 years, 1 month 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 ...
5 years, 1 month 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. ...
5 years, 1 month 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. ...
5 years, 1 month ago (2014-03-13 01:04:25 UTC) #6
dfc
>> No. GetQueuedCompletionStatus won't do, we need >> GetQueuedCompletionStatusEx. See netpoll_windows.c for details. > > ...
5 years, 1 month 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 > >> ...
5 years, 1 month 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: ...
5 years, 1 month 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 ...
5 years, 1 month 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 ...
5 years, 1 month 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 ...
5 years, 1 month 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 ...
5 years, 1 month 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 ...
5 years, 1 month 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 ...
5 years, 1 month 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 ...
5 years, 1 month 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: > > > > ...
5 years, 1 month 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?) ...
5 years ago (2014-03-25 00:35:15 UTC) #18
r
You have an LGTM from alex. -rob
5 years 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 ...
5 years 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 ...
5 years 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
5 years 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 // ...
5 years 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 ...
5 years 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 ...
5 years ago (2014-03-25 14:58:05 UTC) #25
brainman
5 years 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 f62528b